Add File / FileReader polyfill by bkaradzic-microsoft · Pull Request #169 · BabylonJS/JsRuntimeHost

bkaradzic-microsoft · 2026-05-26T23:32:52Z

The File polyfill in BabylonNative currently lives as a JS shim (Apps/Playground/Scripts/file_polyfill.js, ~308 lines) that is LoadScript-ed from the Playground's AppContext (see BabylonJS/BabylonNative#1706). Per @bghgary's review on that PR, it should live in JsRuntimeHost alongside Blob, URL, WebSocket, XMLHttpRequest, TextDecoder, AbortController, etc., so every JsRuntimeHost consumer -- not only the BabylonNative Playground -- gets File and FileReader.

This PR adds a native C++ Polyfills/File/ target that implements the WHATWG File and FileReader web APIs, plus 29 new Mocha unit tests (15 File + 14 FileReader) under Tests/UnitTests/Scripts/tests.ts.

Surface area

File(parts, name, options) constructor.
File instance accessors: size, type, name, lastModified.
File instance methods: arrayBuffer(), text(), bytes().
File extends Blob: the prototype chain is wired so new File(...) instanceof Blob === true (Babylon.js core branches on instanceof Blob in fileTools, Offline/database, abstractEngine, thinNativeEngine).
FileReader with state constants EMPTY/LOADING/DONE (exposed both on the constructor and on instances, per WHATWG).
FileReader readonly attributes readyState, result, error and the onloadstart/onprogress/onload/onabort/onerror/onloadend EventHandler attributes.
FileReader events delivered via both the onX handler slots and addEventListener / removeEventListener / dispatchEvent.
FileReader.readAsText / readAsArrayBuffer / readAsDataURL / abort.

Implementation notes

File delegates its byte storage to the existing Blob polyfill via env.Global().Get("Blob"), reusing Blob's BlobPart handling (ArrayBuffer, typed array, string, Blob). Blob::Initialize must run before File::Initialize.
FileReader's readonly attributes (readyState/result/error) and the on* handler slots are backed by C++ state surfaced through prototype accessors, not by writable JS properties, so script can neither overwrite them nor fool the state-machine checks. result/error are boxed on a persistent holder object rather than Napi::Reference<Value> slots, because napi_create_reference rejects primitive values on the real N-API backends (V8/JSC) and readAsText/readAsDataURL produce primitive string results.
readAsDataURL base64-encodes via the base-n dependency (added as a top-level FetchContent), matching the encoder already used by other polyfills.
An in-flight read keeps the FileReader alive (per spec) by anchoring its JS wrapper in a shared_ptr<Napi::ObjectReference> captured by the promise reactions -- externally owned, released automatically when the promise settles. A monotonic read id lets abort() / restart invalidate a stale continuation so a late-resolving arrayBuffer() promise cannot dispatch a phantom load after a user-initiated abort.
The new JSRUNTIMEHOST_POLYFILL_FILE CMake option is on by default and gates building the target.

Notable JSC-specific care

Two pieces of the implementation are shaped specifically to work correctly on JavaScriptCore:

FileReader.EMPTY/LOADING/DONE are registered via StaticValue and InstanceValue descriptors inside the DefineClass property list, not via func.Get("prototype").Set(...) after class creation. On JSC the napi shim's ConstructorInfo::Create defaults the constructor's .prototype to Object.prototype, so the latter pattern would pollute Object.prototype with EMPTY/LOADING/DONE keys and break for..in over plain objects throughout the runtime. The regression test "does not pollute Object.prototype with EMPTY/LOADING/DONE" pins this behaviour.
The Blob-dependency guard in File::Initialize uses IsUndefined() rather than IsFunction(). Some JSC builds (notably libjavascriptcoregtk-4.1 on Linux) classify constructors created via JSObjectMakeConstructor as typeof 'object', not 'function', so napi_typeof returns napi_object for them and an IsFunction() guard would silently early-return on those engines. IsUndefined() matches the guard used by Blob and works on V8, JSI, Chakra, iOS-JSC, Android-JSC, and Linux JSC.

Testing

Locally UnitTests.exe (Win32 x64 Chakra Debug) passes all 180 JS tests (151 pre-existing + 29 new) and the native gtests, including the Object.prototype-pollution regression canary and the abort / late-resolve continuation guards. CI is green across all engines/platforms (V8, Chakra, JSI/JSC, iOS, Android, UWP, macOS, Ubuntu, sanitizers).

After this merges

BabylonJS/BabylonNative#1706 re-pins CMakeLists.txt's GIT_REPOSITORY/GIT_TAG to the merged SHA and removes the JS shim.

Copilot

Pull request overview

This PR moves File / FileReader support from a BabylonNative Playground-only JS shim into JsRuntimeHost as a native C++ polyfill, so all JsRuntimeHost consumers get the WHATWG-style APIs alongside existing polyfills (Blob, URL, XHR, etc.).

Changes:

Add a new Polyfills/File/ C++ target implementing File and FileReader (including event handler slots + EventTarget-style listeners).
Wire the polyfill into the build via a new JSRUNTIMEHOST_POLYFILL_FILE CMake option (ON by default) and link it into unit tests.
Add new Mocha unit tests covering File and FileReader behaviors (construction, reads, events, abort, and a JSC regression around Object.prototype pollution).

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
CMakeLists.txt	Adds `JSRUNTIMEHOST_POLYFILL_FILE` option (ON by default).
Polyfills/CMakeLists.txt	Conditionally includes the new `File` polyfill subdirectory.
Polyfills/File/CMakeLists.txt	Defines the `File` polyfill library target and its sources.
Polyfills/File/Include/Babylon/Polyfills/File.h	Public API entrypoint for initializing the polyfill.
Polyfills/File/Readme.md	Documents behavior and prerequisites (Blob must be initialized first).
Polyfills/File/Source/File.h	Declares the internal `File` ObjectWrap implementation.
Polyfills/File/Source/File.cpp	Implements `File` over the existing `Blob` polyfill and initializes `FileReader`.
Polyfills/File/Source/FileReader.h	Declares the internal `FileReader` ObjectWrap implementation.
Polyfills/File/Source/FileReader.cpp	Implements async reads, base64 DataURL generation, events, and abort logic.
Tests/UnitTests/CMakeLists.txt	Links the new `File` target into the UnitTests executable.
Tests/UnitTests/Shared/Shared.cpp	Initializes the `File` polyfill in the unit test JS environment.
Tests/UnitTests/Scripts/tests.ts	Adds new Mocha tests for `File` and `FileReader`, including a JSC regression canary.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

bkaradzic-microsoft · 2026-06-01T21:42:14Z

@bghgary friendly ping — this is the upstream half of BabylonJS/BabylonNative#1706 (File / FileReader polyfill, which re-enables 19 GLTF/OBJ serializer tests in BN). Same flow as #171 (TextEncoder) which you helped me close out earlier today: the polyfill was moved here from the BN PR per your review ("this should be a proper C++ polyfill under Polyfills/ so every consumer gets it").

State just refreshed:

Rebased onto current upstream/main (resolved a trivial overlapping-describe() conflict in tests.ts against the just-merged Add TextEncoder polyfill (WHATWG Encoding Standard) #171).
5 inline Copilot review comments all have my replies with fixes in commit 52f6338. Notable bug-fix: readAsBinaryString now produces Latin-1 byte-per-code-unit per spec (was emitting UTF-8 of code points ≥ 0x80).
File library builds standalone clean (cmake --build build-notests --target File).

Polyfills/File/ sits next to Polyfills/Blob/ and follows the same shape as TextDecoder / TextEncoder / XMLHttpRequest / AbortController / URL / WebSocket. No changes to existing files outside the standard wiring (CMakeLists.txt option, Polyfills/CMakeLists.txt add_subdirectory, Tests/UnitTests/{CMakeLists, Shared/Shared.cpp, Scripts/tests.ts} link + Initialize + tests).

Once this lands I'll bump BN #1706's pin back to BabylonJS/JsRuntimeHost and that PR is one tiny commit away from merge.

ryantrem

Similar to my comment on the other PR, we may want to do a minimal implementation of the File polyfill that is just enough to support BJS usage (not sure if that is already the case).

@ryantrem

…_ptr trick Addresses @ryantrem review on BabylonJS#169 (review id 4412098338): * Drop FileReader.readAsBinaryString and its supporting code paths. BJS has zero call sites (searched core, dev/, tools/). Removing it also eliminates the deferred Latin-1/UTF-8 follow-up that previously crashed Chakra during a combined-edit attempt. * Wire File.prototype to inherit from Blob.prototype via Object.setPrototypeOf in File::Initialize. Babylon.js core branches on `instanceof Blob` in fileTools, Offline/database, abstractEngine, and thinNativeEngine; without inheritance those checks silently fail for File inputs and the wrong branch is taken. Internal m_blob composition stays as the implementation detail; only the JS-visible prototype chain is wired. New test asserts `new File(...) instanceof Blob === true`. * Replace shared_ptr<ObjectReference>-in-lambda trick with a member Napi::ObjectReference m_selfRef anchored across the in-flight read. Matches the member-slot pattern used by WebSocket/XHR in this repo. Lambdas now only capture POD + this, so they remain copyable for jsi::Function's std::function-style callable slot. Every terminal path (load, error, abort) resets m_selfRef to break the self-cycle. Each lambda also guards on m_readId before dereferencing m_selfRef to handle the abort-then-late-resolve race.

bkaradzic-microsoft · 2026-06-02T20:22:14Z

@ryantrem on the minimization point — I surveyed Babylon.js usage and the only thing that wasn't already minimal is FileReader.readAsBinaryString (zero call sites across packages/dev, packages/tools, etc.). Dropped it in 4480ad8.

API	BJS usage
`new File(parts, name, opts)`	inspector-v2, viewer, smartAssetSerializer, etc.
`instanceof File`	modelLoader, smartAssetSerializer, configurator
`instanceof Blob`	fileTools, Offline/database, abstractEngine, thinNativeEngine
`FileReader.readAsArrayBuffer`	fileTools, database
`FileReader.readAsText`	fileTools, smartAssetSerializer
`FileReader.readAsDataURL`	stringTools, nodeEditor, inspector
`FileReader.readAsBinaryString`	zero call sites — removed

Dropping readAsBinaryString also makes the deferred Latin-1/UTF-8 follow-up unnecessary (the one that previously crashed Chakra during a combined-edit attempt — see the earlier thread).

Full set of changes in 4480ad8:

Drop readAsBinaryString + its enum case + its test.
File inherits Blob via prototype-chain wire-up + new instanceof Blob test.
Replace shared_ptr<ObjectReference> with member Napi::ObjectReference m_selfRef slot (matches the WebSocket/XHR pattern in this repo).

Local Win32 Chakra File target builds clean.

bkaradzic-microsoft · 2026-06-02T21:25:37Z

CI on 35bfa81 is fully green (22/22 jobs across V8, Chakra, JSI, JSC on Linux/macOS/iOS/Android/Windows/UWP, plus Sanitizers + TSan).

The prototype-chain wire-up for File extends Blob went through three iterations:

4480ad8 (initial review-fix commit): direct Object.setPrototypeOf(func.prototype, blobProto) in C++. Passed V8/Chakra; failed JSC because JSC's napi_define_class (which wraps JSObjectMakeConstructor) returns Object.prototype from func.Get("prototype") rather than the real napi-internal prototype — same quirk the FileReader constants block documents. Result: setPrototypeOf tried to mutate Object.prototype's [[Prototype]] and threw TypeError.
ac19d77: temp-instance trick — Object.getPrototypeOf(new File(...)) returns JSC's real napi-internal prototype, and that one accepts setPrototypeOf. Fixed JSC; broke Chakra (reason still unclear from the log alone — Chakra-specific behavior somewhere between JsConstructObject and Object.getPrototypeOf).
9e7b333 (C++ dual-path direct + verify + fallback): regressed JSC because the JSC napi shim escapes the direct-path TypeError as [Uncaught Error] instead of capturing it via IsExceptionPending, so the C++ catch never gets the chance to fall through to the fallback.
35bfa81 (current): move both paths into a small JS shim wrapped in try/catch. JS-level catch reliably traps the TypeError on every engine; direct path covers V8/Chakra; probe path (instanceof Blob check + getPrototypeOf + setPrototypeOf on the real napi-internal prototype) covers JSC. The shim is run once at Initialize via env.RunScript (Shared N-API) or Napi::Eval (JSI N-API).

Ball back in your court for re-review.

bghgary

[Reviewed by Copilot on behalf of @bghgary]

Recommend landing #172 first. The JSC napi-shim bug it describes is the root cause of several workarounds in this PR — JS_PROTOTYPE_CHAIN_SHIM collapses to one direct setPrototypeOf call, the Napi::Eval / env.RunScript dispatch goes away, and the 14-line FileReader dual-exposure comment shrinks to one line. Quite a bit of code evaporates before merge if #172 lands first.

Other concerns inline.

bkaradzic-microsoft · 2026-06-02T23:56:23Z

Filed #174 with the JSC napi shim fix for #172. It replaces the old // BEGIN TODO ... END TODO [[Prototype]]-kludge in ConstructorInfo::Create with a proper .prototype property setup, and stops napi_define_class from going through napi_get_prototype (which per spec returns [[Prototype]], not .prototype). Includes a Blob-based regression test that asserts no instance members leak onto the global Object.prototype.

Once #174 lands, I'll rebase this branch onto main and:

Collapse JS_PROTOTYPE_CHAIN_SHIM + the Napi::Eval / env.RunScript dispatch into a single direct C++ Object.setPrototypeOf(File.prototype, Blob.prototype) call (your Android NDK25 #51 + Support for POST requests and setting request headers #12 asks).
Drop the 14-line FileReader StaticValue/InstanceValue comment block per Web polyfill constants: add missing InstanceValue exposure on WebSocket and XMLHttpRequest #173 (your Small inclusion fix for <exception> #109 ask).
Address the remaining inline asks: dangling comment at FileReader.cpp:60 (your UWP isn't currently able to work with V8 #60), InstanceAccessor getters/setters for readyState/result/error + on* handlers (your JSC fix utf8 string creation #140, Fix WorkQueue destructor deadlock #147), early-out + throw-on-missing-Blob reordering in File::Initialize (your Expose napi_add_finalizer #75), promote azawadzki/base-n from BabylonNative to JsRH for base64 (your Build instructions for Android #15), file a Chakra throw-from-constructor tracker issue (your #1479).

Will respond on the shared_ptr thread separately — your UAF argument is right; reverting the m_selfRef simplification.

Re: splitting tests.ts per-polyfill (your #1400) — happy to do it in this PR or as a follow-up; let me know which you prefer.

bkaradzic-microsoft · 2026-06-03T00:04:11Z

Update: closing #174 without merging — the JSObjectMakeConstructor .prototype slot is ReadOnly per JSObjectRef.cpp:128, so JSObjectSetProperty calls put() on it and throws on every JSC backend. The naive workaround JSObjectMakeFunctionWithCallback produces a non-constructable result. The actually-correct fix needs to switch to JSObjectMake + JSClassDefinition with both callAsConstructor and callAsFunction, plus a refactor of the NativeInfo::Link / Query sentinel pattern — too big to bundle with this PR.

So #169 keeps its JS-shim wire-up (35bfa81). I'll address your remaining inline asks against the current shape:

UWP isn't currently able to work with V8 #60 dangling FileReader.cpp comment — fix as suggested.
Small inclusion fix for <exception> #109 drop the 14-line dual-exposure comment block per Web polyfill constants: add missing InstanceValue exposure on WebSocket and XMLHttpRequest #173 (Web polyfill constants: add missing InstanceValue exposure on WebSocket and XMLHttpRequest #173 is fine to land independently as the WHATWG pattern; the JSC pitfall stays, just keep one line that points at it).
JSC fix utf8 string creation #140 / Fix WorkQueue destructor deadlock #147 readyState/result/error/on* → InstanceAccessor getters/setters with C++-side state. This is the meatiest refactor; will require touching 7 jsThis.Set + 4 jsThis.Get sites.
Build instructions for Android #15 promote azawadzki/base-n from BabylonNative to JsRH; FileReader consumes it. (Saves a third base64 impl; allows BN to drop the direct dep in a follow-up.)
Expose napi_add_finalizer #75 reorder File::Initialize (early-out first, throw on missing Blob).
Support for POST requests and setting request headers #12 clean up the misleading BABYLON_POLYFILL_USE_NAPI_JSI_EVAL macro (pick env.RunScript or Napi::Eval, drop the #if).
#1479 file a JRH tracker for the Chakra throw-from-constructor limitation; reference it in the TODO.
shared_ptr UAF revert — you're right, restoring std::shared_ptr<Napi::ObjectReference> per-lambda + dropping m_selfRef.
#1400 tests.ts split — happy to do either way; defaulting to follow-up unless you'd rather see it here.

…abylonJS#169 File.cpp - Drop the misleading BABYLON_POLYFILL_USE_NAPI_JSI_EVAL macro and the __has_include(<napi/env.h>) guard. <napi/env.h> exists on every backend (V8, Chakra, JSC, JSI) and is pulled in transitively via <Babylon/Polyfills/File.h>, so the include guard always succeeded and the macro name implied a JSI-only path that doesn't exist — Napi::Eval is declared on all four backends (the Shared N-API impl in env.cc is a thin wrapper around env.RunScript). Call Napi::Eval directly. - Reorder Initialize: check "already provided" (cheap no-op, common path on platforms with a native File) before probing for Blob. - Throw Napi::Error on missing Blob instead of silently bailing. Consumers that wire up the File polyfill expect it to be installed; silent failures are hard to debug. FileReader.cpp - MakeEvent: replace the dangling "JS polyfill that this C++ implementation replaces" reference (no such JS polyfill exists in this PR) with a one-line description of the actual ProgressEvent contract. - DefineClass: collapse the 14-line dual-StaticValue/InstanceValue comment to one line pointing at JsRH#173. Per BabylonJS#173 this is the correct WHATWG IDL `const` member exposure pattern, not a workaround that needs in-line justification. tests.ts - Re-point the Chakra throw-from-constructor TODO at JsRH#175 (filed today) so the disabled "throws when fewer than 2 arguments are passed" test can be re-enabled atomically when that shim limitation is fixed. No behavior change; pure cleanup. Local Chakra build clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bkaradzic-microsoft · 2026-06-03T00:21:25Z

Pushed 3de01db addressing the non-controversial mechanical nits from the review. CI run 26855651151 — 22/22 green.

Changes in this commit:

File.cpp: drop the misleading BABYLON_POLYFILL_USE_NAPI_JSI_EVAL macro and __has_include(<napi/env.h>) guard. Napi::Eval is declared on every backend (Shared N-API has it via Core/Node-API/Source/env.cc as a thin wrapper around env.RunScript; JSI N-API declares it directly in <napi/env.h>). The include is also already pulled in transitively via <Babylon/Polyfills/File.h>, so the guard always succeeded. Now a single unconditional Napi::Eval(...) call.
File.cpp Initialize: reorder so the "already provided" check runs first (cheap no-op, common path on hosts with a native File), and throw Napi::Error on missing Blob instead of silently returning. Consumers wiring up the polyfill expect it to be installed — silent failures are hard to diagnose.
FileReader.cpp MakeEvent: drop the dangling "JS polyfill that this C++ implementation replaces" reference (there's no such JS polyfill in this PR); replaced with a one-liner describing the actual ProgressEvent loaded/total contract.
FileReader.cpp DefineClass: collapse the 14-line dual-StaticValue/InstanceValue block down to one line pointing at Web polyfill constants: add missing InstanceValue exposure on WebSocket and XMLHttpRequest #173, since per Web polyfill constants: add missing InstanceValue exposure on WebSocket and XMLHttpRequest #173 that's the correct WHATWG IDL const member exposure pattern, not a workaround needing in-line justification.
tests.ts: re-point the Chakra throw-from-constructor TODO to Chakra napi shim: throws from class constructors are swallowed (don't surface to JS) #175 (filed today) so the disabled expect(() => new (File as any)()).to.throw() test can be re-enabled atomically when the Chakra shim limitation is fixed.

Still to come (separate commits for review-clarity, in priority order):

shared_ptr<Napi::ObjectReference> UAF revert in FileReader.cpp (your #r3344177855 ask) — restoring the per-lambda shared_ptr capture pattern that survives abort → drop → GC → engine-owned promise settles → onResolve fires.
InstanceAccessor refactor for readyState / result / error / on* handlers (your JSC fix utf8 string creation #140 + Fix WorkQueue destructor deadlock #147 asks) — add C++ state members + Napi::FunctionReference slots, replace all the jsThis.Set/.Get sites.
base-n promotion from BN to JRH (your Build instructions for Android #15 ask) — once it lands here, FileReader can drop the inline EncodeBase64 and BN's direct dependency on the same lib goes away in a follow-up.
tests.ts per-polyfill split (your #1400 note that said "your call") — happy to defer unless you'd rather see it now.

#172 / #174 update: as noted in the follow-up comment on #174, the proper JSC .prototype fix needs a JSClassDefinition-based callable+constructable rewrite that's too big to bundle here. The JS shim approach in this PR stays as-is.

@ryantrem

…_ptr trick Addresses @ryantrem review on BabylonJS#169 (review id 4412098338): * Drop FileReader.readAsBinaryString and its supporting code paths. BJS has zero call sites (searched core, dev/, tools/). Removing it also eliminates the deferred Latin-1/UTF-8 follow-up that previously crashed Chakra during a combined-edit attempt. * Wire File.prototype to inherit from Blob.prototype via Object.setPrototypeOf in File::Initialize. Babylon.js core branches on `instanceof Blob` in fileTools, Offline/database, abstractEngine, and thinNativeEngine; without inheritance those checks silently fail for File inputs and the wrong branch is taken. Internal m_blob composition stays as the implementation detail; only the JS-visible prototype chain is wired. New test asserts `new File(...) instanceof Blob === true`. * Replace shared_ptr<ObjectReference>-in-lambda trick with a member Napi::ObjectReference m_selfRef anchored across the in-flight read. Matches the member-slot pattern used by WebSocket/XHR in this repo. Lambdas now only capture POD + this, so they remain copyable for jsi::Function's std::function-style callable slot. Every terminal path (load, error, abort) resets m_selfRef to break the self-cycle. Each lambda also guards on m_readId before dereferencing m_selfRef to handle the abort-then-late-resolve race.

bghgary

[Reviewed by Copilot on behalf of @bghgary]

Please merge main to pick up #177 + #179.

One related cleanup outside this PR's diff — Tests/UnitTests/Scripts/tests.ts lines 1292–1297 still has the TODO(#178) / isPrototypeOf workaround that #177 introduced for Chakra. #179 retired it; after merging main, restore the depth-1 check:

-        // TODO(#178): Chakra's napi_wrap interposes a hidden external object
-        // into the instance's prototype chain, so Object.getPrototypeOf(blob)
-        // !== Blob.prototype at depth 1 on Chakra. Use isPrototypeOf (chain
-        // walk) until that is fixed.
-        expect(Blob.prototype.isPrototypeOf(blob)).to.equal(true);
+        expect(Object.getPrototypeOf(blob)).to.equal(Blob.prototype);

Implements the WHATWG File and FileReader web APIs as a native polyfill, alongside the existing Blob polyfill that File delegates to for byte storage. * `Polyfills/File/` -- new polyfill target, gated on a new `JSRUNTIMEHOST_POLYFILL_FILE` CMake option (ON by default). * `Tests/UnitTests/Scripts/tests.ts` -- 28 new Mocha tests (13 File + 15 FileReader), including a regression canary for Object.prototype pollution by `FileReader.EMPTY/LOADING/DONE`. Notable implementation details ------------------------------ * FileReader registers its `EMPTY/LOADING/DONE` constants via `StaticValue` and `InstanceValue` descriptors inside the `DefineClass` property list. On JavaScriptCore the napi shim's `ConstructorInfo::Create` defaults the constructor's `.prototype` to `Object.prototype`, so setting these via `func.Get("prototype").Set(...)` would pollute every plain object in the runtime. * The Blob-dependency guard in `File::Initialize` uses `IsUndefined()` rather than `IsFunction()`. Some JSC builds (notably `libjavascriptcoregtk` on Linux) classify constructors created via `JSObjectMakeConstructor` as `typeof 'object'`, not `'function'`, so `napi_typeof` returns `napi_object` for them and an `IsFunction()` guard would silently early-return on those engines. `IsUndefined()` matches the guard used by Blob and works on every engine we ship. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…on Chakra ThrowAsJavaScriptException avoids the Chakra heap-corruption on throw-from-constructor, but JSI's napi shim implements `Error::ThrowAsJavaScriptException` as a no-op that only stores the error in `env->last_exception` without raising it. The script then continues normally with an incompletely-initialized File wrapper, the `expect(...).to.throw()` matcher sees no JS error, and the unhandled exception is later reported through AppRuntime's UnhandledExceptionHandler, failing the test harness exit code. Use the JRH-wide convention `throw Napi::TypeError::New(env, msg)` (matches Blob, XHR, URL, AbortSignal, TextDecoder, FileReader). On engines whose Node-API shim properly translates a C++ throw into a JS exception at the ObjectWrap construction boundary (V8 / JSC / JSI), this propagates as expected. Chakra cannot handle throwing from a constructor — there are five pre-existing TODOs in tests.ts noting this same limitation for URL parse() error cases. Apply the same workaround here: keep the WebIDL-conformant throw in C++, but comment out the test that exercises it (it would corrupt the Chakra heap on every CI run). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

@ryantrem

…_ptr trick Addresses @ryantrem review on BabylonJS#169 (review id 4412098338): * Drop FileReader.readAsBinaryString and its supporting code paths. BJS has zero call sites (searched core, dev/, tools/). Removing it also eliminates the deferred Latin-1/UTF-8 follow-up that previously crashed Chakra during a combined-edit attempt. * Wire File.prototype to inherit from Blob.prototype via Object.setPrototypeOf in File::Initialize. Babylon.js core branches on `instanceof Blob` in fileTools, Offline/database, abstractEngine, and thinNativeEngine; without inheritance those checks silently fail for File inputs and the wrong branch is taken. Internal m_blob composition stays as the implementation detail; only the JS-visible prototype chain is wired. New test asserts `new File(...) instanceof Blob === true`. * Replace shared_ptr<ObjectReference>-in-lambda trick with a member Napi::ObjectReference m_selfRef anchored across the in-flight read. Matches the member-slot pattern used by WebSocket/XHR in this repo. Lambdas now only capture POD + this, so they remain copyable for jsi::Function's std::function-style callable slot. Every terminal path (load, error, abort) resets m_selfRef to break the self-cycle. Each lambda also guards on m_readId before dereferencing m_selfRef to handle the abort-then-late-resolve race.

CI failed on JSC engines (Ubuntu_gcc, macOS, iOS, Android_JSC) with '[Uncaught Error] setPrototypeOf@[native code]' during JS env init. Root cause: the previous commit called \Object.setPrototypeOf(func.Get('prototype'), Blob.prototype)\. On JSC, the napi-defined class's \.prototype\ JS property points to Object.prototype (per JSObjectMakeConstructor semantics; same quirk the FileReader constants block documents). setPrototypeOf on Object.prototype throws TypeError because Object.prototype's [[Prototype]] is immutable. Fix: instantiate a throwaway File, fetch its real prototype via Object.getPrototypeOf, and set THAT prototype's prototype to Blob.prototype. On V8 / Chakra the real prototype is also func.prototype, so this is correct everywhere. The temp instance is GC-eligible immediately after. Each call is guarded with IsExceptionPending + GetAndClearPendingException so that if a non-Chromium JSC build still rejects setPrototypeOf on the napi-internal prototype, Initialize stays best-effort: instances won't be Blob subtypes on that engine but the rest of the polyfill still installs and the JS env starts cleanly.

Commit ac19d77 swapped the direct setPrototypeOf for a temp-instance trick to fix JSC, but that broke Chakra (Win32_x64_Chakra and Win32_x86_Chakra now report `file instanceof Blob === false`). Restore the direct call (which previously passed on V8 and Chakra), verify the chain via a probe instance, and only fall back to the temp-instance approach when the chain isn't wired up. That recovers JSC (where `func.prototype` aliases `Object.prototype`) without regressing the engines where the direct call was already correct.

Commit 9e7b333 (C++ dual-path direct -> temp-instance fallback) passed Chakra but regressed JSC: on JSC, setPrototypeOf on Object.prototype throws TypeError that escapes the napi shim as an [Uncaught Error] instead of being capturable via IsExceptionPending. Move both setPrototypeOf calls into a single JS shim wrapped in try/catch. JS-level try/catch reliably traps the TypeError on every engine, the direct path takes care of V8/Chakra, and the probe path (instanceof Blob check + getPrototypeOf + setPrototypeOf on the real napi-internal prototype) takes care of JSC.

…abylonJS#169 File.cpp - Drop the misleading BABYLON_POLYFILL_USE_NAPI_JSI_EVAL macro and the __has_include(<napi/env.h>) guard. <napi/env.h> exists on every backend (V8, Chakra, JSC, JSI) and is pulled in transitively via <Babylon/Polyfills/File.h>, so the include guard always succeeded and the macro name implied a JSI-only path that doesn't exist — Napi::Eval is declared on all four backends (the Shared N-API impl in env.cc is a thin wrapper around env.RunScript). Call Napi::Eval directly. - Reorder Initialize: check "already provided" (cheap no-op, common path on platforms with a native File) before probing for Blob. - Throw Napi::Error on missing Blob instead of silently bailing. Consumers that wire up the File polyfill expect it to be installed; silent failures are hard to debug. FileReader.cpp - MakeEvent: replace the dangling "JS polyfill that this C++ implementation replaces" reference (no such JS polyfill exists in this PR) with a one-line description of the actual ProgressEvent contract. - DefineClass: collapse the 14-line dual-StaticValue/InstanceValue comment to one line pointing at JsRH#173. Per BabylonJS#173 this is the correct WHATWG IDL `const` member exposure pattern, not a workaround that needs in-line justification. tests.ts - Re-point the Chakra throw-from-constructor TODO at JsRH#175 (filed today) so the disabled "throws when fewer than 2 arguments are passed" test can be re-enabled atomically when that shim limitation is fixed. No behavior change; pure cleanup. Local Chakra build clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…onJS#177 landed BabylonJS#177 fixed the JSC napi shim's Object.prototype pollution by passing the per-class JSClassRef to JSObjectMakeConstructor, so napi-defined classes now get a real per-class .prototype object instead of aliasing the global Object.prototype. That removes the entire reason for the JS_PROTOTYPE_CHAIN_SHIM and the Napi::Eval / try/catch dance: we can wire File.prototype's [[Prototype]] to Blob.prototype with a direct Object.setPrototypeOf call. Drops ~50 lines of explanatory comment + shim + Eval-with-IsExceptionPending guard in File::Initialize down to a 4-line napi call. `file instanceof Blob` regression coverage remains in tests.ts:1564 and BabylonJS#177's `describe("napi class prototype isolation (BabylonJS#172)")` block at tests.ts:1271 (uses Blob — and File extends Blob — to assert that napi-defined classes don't share Object.prototype). Local Chakra build clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…ment Per bghgary's review nit: the "BabylonJS#177 fixed JSC .prototype pollution" context is now in main's git history, doesn't need to live in the comment block. Comment now focuses on WHY we wire File→Blob, not on which past bug enabled the wiring to be one-liner. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bkaradzic-microsoft · 2026-06-04T00:40:29Z

Rebased onto current main (now includes #177 + #179). fe457e5.

Both asks from your latest review addressed:

Chakra napi_wrap mutates instance [[Prototype]] chain #178 / isPrototypeOf workaround in tests.ts:1292-1297 — auto-resolved by the rebase. Stop mutating instance [[Prototype]] chain in napi_wrap on Chakra #179 already replaced the workaround block with the depth-1 Object.getPrototypeOf(blob) === Blob.prototype check on main, and rebasing pulled it in directly. grep -n 'isPrototypeOf\|#178' in tests.ts returns no matches.
File.cpp historical paragraph nit — applied the suggestion verbatim. The setPrototypeOf comment now sticks to why (WHATWG subtype + BJS-core instanceof Blob branches) and drops the how-we-got-here paragraph.

Rebase was clean — 8 commits replayed cleanly on top of a128e68, no conflicts. CI on the new head should be picking up shortly; will follow up once it lands.

…eAccessor readyState/result/error are WHATWG readonly attributes and the on* slots are EventHandler IDL attributes; both should be prototype accessors, not writable enumerable own data properties stamped onto every instance. - readyState/result/error -> InstanceAccessor getters reading m_readyState / m_result / m_error. State-machine checks now read the C++ members directly, so JS can no longer tamper with them by overwriting the property. - on* handlers -> a single InstanceAccessor get/set pair whose accessor data carries the event type, backed by a Napi::FunctionReference map. Dispatch consults the map instead of the on-prefixed instance property. - Constructor no longer stamps any instance properties. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…impl Per review, avoid carrying a third hand-rolled base64 encoder. Promote the header-only azawadzki/base-n (already used by BabylonNative) into JsRuntimeHost as a FetchContent INTERFACE library, gated on JSRUNTIMEHOST_POLYFILL_FILE, and link it PRIVATE into the File polyfill. base-n's encode_b64 emits unpadded base64, so EncodeBase64 now just delegates to it and appends the RFC 4648 '=' padding for the final partial group. This keeps data: URL output byte-for-byte identical (e.g. "Hello" -> "SGVsbG8="). Pinned to the same commit BabylonNative uses (7573e77c). BN's direct base-n dependency can be dropped as a follow-up once it consumes JRH's. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bkaradzic-microsoft · 2026-06-04T01:04:28Z

Addressed the remaining review asks (pushed eaeaf20, a6155da):

readyState / result / error are now InstanceAccessor getters backed by m_readyState / m_result / m_error. State-machine checks read the C++ members directly, so they can't be tampered with from JS, and the constructor no longer stamps instance data props.
on* handlers collapsed to a single InstanceAccessor get/set pair (accessor data carries the event type) backed by a Napi::FunctionReference map that Dispatch consults directly.
base64: removed the bespoke encoder. Promoted header-only azawadzki/base-n into JRH as a FetchContent INTERFACE lib (gated on JSRUNTIMEHOST_POLYFILL_FILE, pinned to the same 7573e77c commit BabylonNative uses), linked PRIVATE into File. EncodeBase64 now delegates to bn::encode_b64 + RFC 4648 padding; output is byte-for-byte identical. BN's direct base-n dep can be dropped as a follow-up.
shared_ptr thread: already addressed by the member m_selfRef (corrected my earlier note).
tests.ts split: filed Split Tests/UnitTests/Scripts/tests.ts into per-polyfill spec files #180 as a standalone follow-up to keep this diff focused.

All 18 review threads are now resolved.

napi_create_reference only accepts heap values (object/function/symbol) on the V8/JSC N-API backends, so storing a primitive string result (readAsText/readAsDataURL) in a Napi::Reference<Value> threw there and the load/loadend events never fired (Chakra's shim tolerated it, which is why this passed locally). Box result/error as properties on a persistent holder object instead; referencing the holder object is valid for every engine while keeping the state C++-owned and tamper-proof. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bkaradzic-microsoft · 2026-06-04T02:03:32Z

CI on 14122f1 is green across every engine. The earlier 7 FileReader async-read failures were a real regression from the InstanceAccessor refactor: readAsText / readAsDataURL produce a string result, and the previous Napi::Reference<Napi::Value> stored it via napi_create_reference, which only accepts heap values (object/function/symbol) on real V8/JSC N-API. That threw on those backends (timeout on V8, uncaught C++ exception → SIGABRT on JSC/Linux) while Chakra's shim tolerated it. Fixed by boxing result/error as properties on a persistent holder object (Napi::ObjectReference m_state), which is always a valid reference target; state stays C++-owned and tamper-proof.

Verified green (all FileReader tests pass — 179 passing):

V8: Win32_x64_V8, Android_V8, UWP_x64_V8
JSC: Ubuntu_clang, Ubuntu_gcc, macOS_Xcode164_Sanitizers, iOS, Android_JSC
Chakra: Win32_x86, UWP_x64
JSI: Win32_x64_JSI, UWP_x64_JSI, UWP_arm64_JSI
Sanitizers: Ubuntu ASAN/UBSan + ThreadSanitizer, macOS ThreadSanitizer

The only red job is Win32_x64_Chakra, failing solely on the unrelated, pre-existing flaky WebSocket > should connect correctly with one websocket connection test (2000 ms timeout against the external wss://ws.postman-echo.com/raw echo server; the sibling multi-connection test passes). All FileReader tests pass there too.

bghgary

[Reviewed by Copilot on behalf of @bghgary]

Concerns inline.

Replace the m_selfRef member self-reference with a shared_ptr<ObjectReference> captured by both promise reactions. The spec requires an in-flight read to keep the FileReader alive even if script drops its reference; this anchors the wrapper externally (owned by the lambdas) for exactly the read's duration. When the promise settles and the engine releases the reactions, the last shared_ptr copy drops and the anchor is released automatically. This drops the member self-cycle and the manual m_selfRef.Reset() on every terminal path, and removes the dead-this race the self-anchor left on the abort-then-drop path: the wrapper is now guaranteed alive whenever a reaction runs, so reading m_readId and calling anchor->Value() are always safe. m_readId is retained solely as the abort/restart guard. Matches the externally-anchored, lambda-owned-state convention used by the WebSocket polyfill. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

bkaradzic-microsoft · 2026-06-04T20:03:44Z

@bghgary thanks for the careful review — all of your inline concerns are addressed in 61d9924:

m_selfRef self-anchor is gone. Replaced with an externally-anchored shared_ptr<Napi::ObjectReference> captured by both promise reactions, matching the WebSocket lambda-owned-state convention. No self-cycle, no manual Reset() on terminal paths (all four deleted), and no dead-this race — the wrapper is guaranteed alive whenever a reaction runs. m_readId is kept solely as the abort/restart guard. Details + the one spec nuance (an in-flight read must keep the reader alive, so the wrapper can't simply be allowed to die) are in the resolved inline thread.

I also refreshed the PR description to match the current implementation (it had drifted): C++-backed readonly accessors for readyState/result/error and the on* slots, boxed result/error for primitive string results on real N-API, base-n for readAsDataURL (no more inlined encoder), and updated test counts (29 new: 15 File + 14 FileReader; 180/180 JS tests).

Verified Win32-x64 Chakra Debug green (180 JS + native gtests), and full CI is green across all engines/platforms. PTAL.

bghgary

[Reviewed by Copilot on behalf of @bghgary]

LGTM. All concerns addressed; the FileReader in-flight lifecycle/UAF fix is verified in 61d9924 — shared_ptr<ObjectReference> anchor captured by both promise reactions, no self-ref member, no manual Reset().

### Problem When JsRuntimeHost is embedded via FetchContent in a parent project that already declares its own `base-n` INTERFACE target — e.g. **BabylonNative**, which uses base-n for its Window/Canvas polyfills — configuring fails: ``` CMake Error at .../jsruntimehost-src/CMakeLists.txt:150 (add_library): add_library cannot create target "base-n" because another target with the same name already exists. The existing target is an interface library created in source directory ".../Dependencies". ``` This surfaced now because the merged File / FileReader polyfill (#169) switched its base64 encoder over to the `base-n` dependency, and BabylonNative declares `base-n` before it fetches JsRuntimeHost. ### Fix Guard the `base-n` block with `NOT TARGET base-n` so that when a parent project already provides the target, JsRuntimeHost adopts it instead of redeclaring. This is the standard CMake subproject pattern and mirrors BabylonNative's own `if(NOT TARGET glslang)` guards. Standalone JsRuntimeHost builds are unaffected (no pre-existing target → block runs as before). ### Validation - Standalone `NAPI_JAVASCRIPT_ENGINE=Chakra` configure + `UnitTests` build: green, File polyfill + base-n link as before. - Unblocks BabylonJS/BabylonNative#1706 (the consuming PR that re-pins to merged main). Co-authored-by: Branimir Karadzic <branimirkaradzic@gmail.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings May 26, 2026 23:32

Copilot started reviewing on behalf of bkaradzic-microsoft May 26, 2026 23:33 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

Comment thread Polyfills/File/Source/FileReader.cpp Outdated

Comment thread Polyfills/File/Source/FileReader.cpp

Comment thread Polyfills/File/Source/FileReader.cpp Outdated

Comment thread Polyfills/File/Source/File.cpp Outdated

Comment thread Tests/UnitTests/Scripts/tests.ts Outdated

bkaradzic-microsoft force-pushed the weekend/tpc-1582-file-polyfill branch from 6f8a827 to 67f9f5c Compare May 27, 2026 00:00

bkaradzic-microsoft requested a review from bghgary May 27, 2026 00:54

bkaradzic-microsoft force-pushed the weekend/tpc-1582-file-polyfill branch 8 times, most recently from fa84e76 to 52f6338 Compare June 1, 2026 21:41

This was referenced Jun 1, 2026

Bump JsRuntimeHost for File/FileReader polyfill (re-enables 19 GLTF/OBJ tests) BabylonJS/BabylonNative#1706

Merged

Use native fetch() polyfill from JsRuntimeHost in Playground BabylonJS/BabylonNative#1707

Draft

ryantrem reviewed Jun 2, 2026

View reviewed changes

Comment thread Polyfills/File/Source/FileReader.cpp Outdated

Comment thread Polyfills/File/Source/File.h

This was referenced Jun 2, 2026

JSC napi shim: writing to constructor.prototype pollutes global Object.prototype #172

Closed

Web polyfill constants: add missing InstanceValue exposure on WebSocket and XMLHttpRequest #173

Open

bghgary requested changes Jun 2, 2026

View reviewed changes

bkaradzic-microsoft mentioned this pull request Jun 2, 2026

JSC napi: stop polluting global Object.prototype from napi_define_class #174

Closed

bkaradzic-microsoft mentioned this pull request Jun 3, 2026

Chakra napi shim: throws from class constructors are swallowed (don't surface to JS) #175

Open

bghgary reviewed Jun 4, 2026

View reviewed changes

Comment thread Polyfills/File/Source/File.cpp Outdated

bkaradzic and others added 9 commits June 3, 2026 17:39

bkaradzic-microsoft force-pushed the weekend/tpc-1582-file-polyfill branch from 0bff942 to fe457e5 Compare June 4, 2026 00:40

bkaradzic-microsoft requested review from bghgary and ryantrem June 4, 2026 00:49

bkaradzic-microsoft and others added 2 commits June 3, 2026 17:57

bkaradzic-microsoft mentioned this pull request Jun 4, 2026

Split Tests/UnitTests/Scripts/tests.ts into per-polyfill spec files #180

Open

bkaradzic-microsoft enabled auto-merge (squash) June 4, 2026 19:29

bghgary reviewed Jun 4, 2026

View reviewed changes

Comment thread Polyfills/File/Source/FileReader.cpp Outdated

bkaradzic-microsoft requested a review from bghgary June 4, 2026 20:03

bghgary approved these changes Jun 5, 2026

View reviewed changes

bkaradzic-microsoft merged commit 2fc671b into BabylonJS:main Jun 5, 2026
23 checks passed

bkaradzic-microsoft mentioned this pull request Jun 5, 2026

Guard base-n target against parent-project collision #184

Merged

bkaradzic-microsoft mentioned this pull request Jun 6, 2026

Add native fetch() polyfill #188

Open

Conversation

bkaradzic-microsoft commented May 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Surface area

Implementation notes

Notable JSC-specific care

Testing

After this merges

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkaradzic-microsoft commented Jun 1, 2026

Uh oh!

ryantrem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bkaradzic-microsoft commented Jun 2, 2026

Uh oh!

bkaradzic-microsoft commented Jun 2, 2026

Uh oh!

bghgary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bkaradzic-microsoft commented Jun 2, 2026

Uh oh!

bkaradzic-microsoft commented Jun 3, 2026

Uh oh!

bkaradzic-microsoft commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bghgary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkaradzic-microsoft commented Jun 4, 2026

Uh oh!

bkaradzic-microsoft commented Jun 4, 2026

Uh oh!

bkaradzic-microsoft commented Jun 4, 2026

Uh oh!

bghgary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bkaradzic-microsoft commented Jun 4, 2026

Uh oh!

bghgary left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bkaradzic-microsoft commented May 26, 2026 •

edited

Loading

bkaradzic-microsoft commented Jun 3, 2026 •

edited

Loading